Allow monitor options in proc_lib:start_monitor/5
#9804
Conversation
Co-authored-by: Jan Uhlig <[email protected]>
CT Test Results 2 files 97 suites 1h 5m 55s ⏱️ Results for commit 6d9d85b. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
| Mon :: reference(), | ||
| Ret :: term() | {error, Reason :: term()}. | ||
|
|
||
| start_monitor(M,F,A,Timeout,SpawnOpts) when is_atom(M), | ||
| is_atom(F), | ||
| is_list(A) -> | ||
| ?VERIFY_NO_MONITOR_OPT(M, F, A, Timeout, SpawnOpts), | ||
| SpawnOpts1 = ensure_spawn_option(monitor, SpawnOpts), |
There was a problem hiding this comment.
FWIW using [monitor, monitor] or [monitor, {monitor, Opts}] is just fine in spawn_opt so it might be fine to just add this option unconditionally
There was a problem hiding this comment.
Yes, but which one of conflicting options (eg monitor vs {monitor, Opts}) wins is undocumented (currently, it is the last one in the list). So if we want to play strictly by the book ("what's not documented may change, at any time, without notice"), we need to check.
| [SpawnOpt | ensure_spawn_option(Opt, SpawnOpts)]; | ||
| ensure_spawn_option(Opt, []) -> | ||
| [Opt]. | ||
|
|
There was a problem hiding this comment.
Minor: it looks like need to remove extra line here
| after 1000 -> ct:fail(no_down) | ||
| end, | ||
| ok. | ||
|
|
Currently, the spawn options
monitorand{monitor, MonitorOpts}are disallowed in theproc_lib:start*functions. This goes back all the way to R11, a ticket OTP-6345, and this post on the old Erlang ML.For
proc_lib:start_monitor, it makes no sense to forbid monitor options. Or at least, we don't see any benefit in doing so. On the contrary, it prevents "customizing" the monitor (eg custom tags, usage of the newpriorityoption, etc). This PR removes this restriction.For
proc_lib:startandproc_lib:start_link, it would be possible to remove the restriction also, that is, without restoring the buggy behavior that seems to have given raise to OTP-6345.However, in the presence of a
monitoror{monitor, MonitorOpts}spawn option, the return value would change fromRetto{Ret, Mon}, with repercussions viageninto thestart/start_linkfunctions of thegen_*behaviors.This may be ok, and require mostly work on the specs and documentation in the
gen_*behaviors, aside from some adaptions ingen. The bigger question IMO is if and how supervisors should cope with this?This PR also does a few more things than what is listed above, which are related to the new
{link, LinkOpts}spawn option that appeared with the new priority messages. Those are mostly just documentation changes.